-
Notifications
You must be signed in to change notification settings - Fork 1
[Feat/#10] 티어 탭 UI #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedToo many files! This PR contains 201 files, which is 51 over the limit of 150. You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chrin05
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 양이 엄청나네요 너무 고생많으셨습니다
| fun d(msg: String, tag: String = "App") = Napier.d(msg, tag = tag) | ||
| fun e(msg: String, t: Throwable? = null, tag: String = "App") = Napier.e(msg, t, tag) | ||
| fun d(tag: String = "App", msg: String) = Napier.d(msg, tag = tag) | ||
| fun e(tag: String = "App", msg: String, t: Throwable? = null, ) = Napier.e(msg, t, tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 콤마가 남아있습니다 제거해주시면 코드 깔끔하게 유지 될 거 같아요!
| ) { | ||
| composable<Tier> { entry -> | ||
| val resultJson by entry.savedStateHandle | ||
| .getStateFlow<String?>("tier_result_json", null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
savedStateHandle의 키값을 상수로 지정해주면 유지보수성을 높여줄 것 같습니다. 만약 변경하게 된다면 아래 36번줄에도 적용할 수 있을 것 같아요
private const val RESULT_FILTER = "tier_result_json"
| onBack = { onBackButtonClick() }, | ||
| onApply = { result -> popBackStackWithResult(result) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 람다 생성 없이 함수만 바로 넘겨줄 수 있을 것 같습니다.
람다 내 인자의 타입 개수 등 모두 일치한다면 아래처럼 사용 가능합니다!
onBack = onBackButtonClick, onApply = popBackStackWithResult,
| sealed interface UiState<out T> { | ||
| data object Loading : UiState<Nothing> | ||
| data class Success<T>(val data: T) : UiState<T> | ||
| data class Failure(val message: String) : UiState<Nothing> | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 대부분의 화면에 적용되어 사용될 거 같은데 core로 빼서 사용할 수 있게 하는 것은 혹시 어떤가요?
| Column( | ||
| modifier = modifier | ||
| .fillMaxSize() | ||
| .background(Color.White) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로는 한 파일 안에 300줄 이상이 되면 가독성을 고려해서 컴포넌트 파일로 분리하는 편인데, 재사용을 하는 컴포넌트 개념이 아니더라도 큰단위에 섹션을 나눠주는 것도 좋을 거 같아요!

개요
#10
PR 유형
해당하는 항목에 체크해주세요.
변경 사항
📸 화면 / 영상 (선택)
tier_tab_iOS.mp4
tier_tab_android.mp4